Skip to content

Conversation

@nfelt
Copy link
Contributor

@nfelt nfelt commented Apr 13, 2021

This is a workaround for #4862 - see that issue for all the gory details.

The tl;dr is that we want to fix an omission in the TF python configuration rules, where they only list a small number of environment variables that their repository rules are sensitive to. In particular they don't include PATH (or PYTHONPATH or PYTHONHOME), even though these are all actually used by the toolchain resolution rules to find the active python binary and python lib directory. So we just patch them in, which ameliorates #4862. (In theory we could try to upstream this, but we're currently pinned to TF 2.3.0 rules and having to upgrade will probably be painful.)

Tested: ran repro script from #4862 and verified that after this change, while it still bakes the local python path into the py_binary wrapper script that's invoked by the genrule, it at least regenerates the wrapper when PATH changes (i.e. when switching virtualenvs) which mostly sidesteps the problem. It would still feel more foolproof to never bake in that path at all and continue using Bazel's autodetecting python toolchain, but to do that I think we might need to migrate off of the TF workspace rules entirely.

@nfelt nfelt requested a review from wchargin April 13, 2021 23:14
Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I insist.

@nfelt nfelt merged commit a5910b1 into master Apr 13, 2021
@nfelt nfelt deleted the nfelt-pygenrule branch April 13, 2021 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants